Skip to content

Step 1 (PR-G): Add grails-code-analysis convention plugin + violation aggregation#15686

Draft
jamesfredley wants to merge 11 commits into
8.0.xfrom
feat/grails-code-analysis-plugin
Draft

Step 1 (PR-G): Add grails-code-analysis convention plugin + violation aggregation#15686
jamesfredley wants to merge 11 commits into
8.0.xfrom
feat/grails-code-analysis-plugin

Conversation

@jamesfredley
Copy link
Copy Markdown
Contributor

Step 1 (PR-G): grails-code-analysis convention plugin + violation aggregation

Carved out of the Hibernate 7 Step 1 PR (#15654) per review feedback from @matrei that the static-analysis tooling should be introduced in its own focused PR.

What this adds

  • grails-code-analysis convention plugin (PMD + SpotBugs) applied to 96 subprojects, with GrailsCodeAnalysisExtension.
  • grails-violation-aggregation root-only plugin that consolidates per-module Checkstyle/CodeNarc/PMD XML reports into Markdown summaries under build/reports/violations/ via an aggregateStyleViolations task.
  • grails-code-style plugin improvements split cleanly from analysis: enable/disable properties, a codenarcFix task, a test-styling toggle, and a resource-path rename to grails-code-style (PMD config under grails-code-analysis).
  • codeanalysis.yml workflow; codestyle.yml switched to the aggregated report format.
  • pmdVersion / spotbugsPluginVersion properties and the violation-fixer skill.

Notes

  • JaCoCo coverage is intentionally not in this PR. It layers on top of the grails-violation-aggregation plugin introduced here and is carved out separately (PR-H).
  • Verified with cd build-logic && ../gradlew :build-logic:test (plugin Spock specs pass).

Why

Splitting this from #15654 keeps the Hibernate 7 baseline clone focused on the actual h5 -> h7 work and lets the project review/decide on the analysis tooling independently.

Assisted-by: claude-code:claude-4.7-opus

Splits static code analysis out of the code-style plugin into a dedicated
grails-code-analysis convention plugin (PMD + SpotBugs), and adds a
root-only grails-violation-aggregation plugin that consolidates the
per-module Checkstyle/CodeNarc/PMD XML reports into Markdown summaries
under build/reports/violations/ via an aggregateStyleViolations task.

Extracted from the Hibernate 7 staging branch (PR #15654) so it can be
reviewed as a focused, single-topic change against 8.0.x, per review
feedback that the analysis tooling should land in its own PR.

Includes:

- GrailsCodeAnalysisPlugin + GrailsCodeAnalysisExtension
- GrailsViolationAggregationPlugin (aggregateStyleViolations)
- GrailsCodeStylePlugin improvements: enable/disable properties,
  codenarcFix task, test-styling toggle; resource path renamed to
  grails-code-style, PMD config under grails-code-analysis
- codeanalysis.yml workflow; codestyle.yml switched to aggregated reports
- pmd/spotbugs versions; violation-fixer skill
- Applies grails-code-analysis to 96 subprojects and wires forge

Assisted-by: claude-code:claude-4.7-opus
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces new Gradle convention plugins to standardize static analysis (PMD + SpotBugs) and to aggregate style/analysis violations into Markdown summaries under build/reports/violations/, along with CI workflow updates to publish those summaries.

Changes:

  • Add grails-code-analysis convention plugin (PMD + SpotBugs, opt-in via properties) and apply it across many subprojects.
  • Add grails-violation-aggregation root-only plugin to generate Markdown violation summaries (and JaCoCo CSV aggregation support).
  • Update GitHub Actions workflows to run aggregation tasks and publish Markdown reports; add PMD/SpotBugs version properties.

Reviewed changes

Copilot reviewed 112 out of 116 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
grails-wrapper/build.gradle Apply grails-code-analysis plugin
grails-web-url-mappings/build.gradle Apply grails-code-analysis plugin
grails-web-mvc/build.gradle Apply grails-code-analysis plugin
grails-web-databinding/build.gradle Apply grails-code-analysis plugin
grails-web-core/build.gradle Apply grails-code-analysis plugin
grails-web-common/build.gradle Apply grails-code-analysis plugin
grails-web-boot/build.gradle Apply grails-code-analysis plugin
grails-views-markup/build.gradle Apply grails-code-analysis plugin
grails-views-gson/build.gradle Apply grails-code-analysis plugin
grails-views-core/build.gradle Apply grails-code-analysis plugin
grails-validation/build.gradle Apply grails-code-analysis plugin
grails-url-mappings/build.gradle Apply grails-code-analysis plugin
grails-testing-support-web/build.gradle Apply grails-code-analysis plugin
grails-testing-support-views-gson/build.gradle Apply grails-code-analysis plugin
grails-testing-support-mongodb/build.gradle Apply grails-code-analysis plugin
grails-testing-support-http-client/build.gradle Apply grails-code-analysis plugin
grails-testing-support-dbcleanup-postgresql/build.gradle Apply grails-code-analysis plugin
grails-testing-support-dbcleanup-h2/build.gradle Apply grails-code-analysis plugin
grails-testing-support-dbcleanup-core/build.gradle Apply grails-code-analysis plugin
grails-testing-support-datamapping/build.gradle Apply grails-code-analysis plugin
grails-testing-support-core/build.gradle Apply grails-code-analysis plugin
grails-test-suite-base/build.gradle Apply grails-code-analysis plugin
grails-test-core/build.gradle Apply grails-code-analysis plugin
grails-spring/build.gradle Apply grails-code-analysis plugin
grails-shell-cli/build.gradle Apply grails-code-analysis plugin
grails-services/build.gradle Apply grails-code-analysis plugin
grails-scaffolding/build.gradle Apply grails-code-analysis plugin
grails-rest-transforms/build.gradle Apply grails-code-analysis plugin
grails-mimetypes/build.gradle Apply grails-code-analysis plugin
grails-logging/build.gradle Apply grails-code-analysis plugin
grails-interceptors/build.gradle Apply grails-code-analysis plugin
grails-i18n/build.gradle Apply grails-code-analysis plugin
grails-gsp/plugin/build.gradle Apply grails-code-analysis plugin
grails-gsp/grails-web-taglib/build.gradle Apply grails-code-analysis plugin
grails-gsp/grails-web-jsp/build.gradle Apply grails-code-analysis plugin
grails-gsp/grails-web-gsp/build.gradle Apply grails-code-analysis plugin
grails-gsp/grails-web-gsp-taglib/build.gradle Apply grails-code-analysis plugin
grails-gsp/grails-taglib/build.gradle Apply grails-code-analysis plugin
grails-gsp/grails-sitemesh3/build.gradle Apply grails-code-analysis plugin
grails-gsp/grails-layout/build.gradle Apply grails-code-analysis plugin
grails-gsp/core/build.gradle Apply grails-code-analysis plugin
grails-gradle/tasks/build.gradle Apply grails-code-analysis plugin
grails-gradle/plugins/build.gradle Apply grails-code-analysis plugin
grails-gradle/model/build.gradle Apply grails-code-analysis plugin
grails-gradle/common/build.gradle Apply grails-code-analysis plugin
grails-geb/build.gradle Apply grails-code-analysis plugin
grails-forge/gradle/code-style-config.gradle Apply grails-code-analysis plugin in Forge build
grails-forge/build.gradle Apply grails-violation-aggregation plugin (Forge root)
grails-fields/build.gradle Apply grails-code-analysis plugin
grails-events/transforms/build.gradle Apply grails-code-analysis plugin
grails-events/spring/build.gradle Apply grails-code-analysis plugin
grails-events/rxjava3/build.gradle Apply grails-code-analysis plugin
grails-events/rxjava2/build.gradle Apply grails-code-analysis plugin
grails-events/rxjava/build.gradle Apply grails-code-analysis plugin
grails-events/plugin/build.gradle Apply grails-code-analysis plugin
grails-events/gpars/build.gradle Apply grails-code-analysis plugin
grails-events/core/build.gradle Apply grails-code-analysis plugin
grails-encoder/build.gradle Apply grails-code-analysis plugin
grails-domain-class/build.gradle Apply grails-code-analysis plugin
grails-datastore-web/build.gradle Apply grails-code-analysis plugin
grails-datastore-core/build.gradle Apply grails-code-analysis plugin
grails-datastore-async/build.gradle Apply grails-code-analysis plugin
grails-datasource/build.gradle Apply grails-code-analysis plugin
grails-datamapping-validation/build.gradle Apply grails-code-analysis plugin
grails-datamapping-tck/build.gradle Apply grails-code-analysis plugin
grails-datamapping-support/build.gradle Apply grails-code-analysis plugin
grails-datamapping-rx/build.gradle Apply grails-code-analysis plugin
grails-datamapping-core/build.gradle Apply grails-code-analysis plugin
grails-datamapping-async/build.gradle Apply grails-code-analysis plugin
grails-databinding/build.gradle Apply grails-code-analysis plugin
grails-databinding-core/build.gradle Apply grails-code-analysis plugin
grails-data-simple/build.gradle Apply grails-code-analysis plugin
grails-data-mongodb/gson-templates/build.gradle Apply grails-code-analysis plugin
grails-data-mongodb/grails-plugin/build.gradle Apply grails-code-analysis plugin
grails-data-mongodb/ext/build.gradle Apply grails-code-analysis plugin
grails-data-mongodb/core/build.gradle Apply grails-code-analysis plugin
grails-data-mongodb/bson/build.gradle Apply grails-code-analysis plugin
grails-data-mongodb/boot-plugin/build.gradle Apply grails-code-analysis plugin
grails-data-hibernate5/spring-orm/build.gradle Apply grails-code-analysis plugin
grails-data-hibernate5/grails-plugin/build.gradle Apply grails-code-analysis plugin
grails-data-hibernate5/dbmigration/build.gradle Apply grails-code-analysis plugin
grails-data-hibernate5/core/build.gradle Apply grails-code-analysis plugin
grails-data-hibernate5/boot-plugin/build.gradle Apply grails-code-analysis plugin
grails-core/build.gradle Apply grails-code-analysis plugin
grails-converters/build.gradle Apply grails-code-analysis plugin
grails-controllers/build.gradle Apply grails-code-analysis plugin
grails-console/build.gradle Apply grails-code-analysis plugin
grails-common/build.gradle Apply grails-code-analysis plugin
grails-codecs/build.gradle Apply grails-code-analysis plugin
grails-codecs-core/build.gradle Apply grails-code-analysis plugin
grails-cache/build.gradle Apply grails-code-analysis plugin
grails-bootstrap/build.gradle Apply grails-code-analysis plugin
grails-async/rxjava3/build.gradle Apply grails-code-analysis plugin
grails-async/rxjava2/build.gradle Apply grails-code-analysis plugin
grails-async/rxjava/build.gradle Apply grails-code-analysis plugin
grails-async/plugin/build.gradle Apply grails-code-analysis plugin
grails-async/gpars/build.gradle Apply grails-code-analysis plugin
grails-async/core/build.gradle Apply grails-code-analysis plugin
gradle.properties Add pmdVersion / spotbugsPluginVersion properties
build.gradle Apply grails-violation-aggregation at root
build-logic/plugins/src/test/groovy/org/apache/grails/buildsrc/GrailsViolationAggregationPluginSpec.groovy Add TestKit coverage for aggregation plugin
build-logic/plugins/src/test/groovy/org/apache/grails/buildsrc/GrailsCodeStylePluginSpec.groovy Add TestKit coverage for codenarcFix behavior
build-logic/plugins/src/main/resources/META-INF/org.apache.grails.buildsrc.grails-code-style/codenarc/codenarc.groovy Add default CodeNarc ruleset resource
build-logic/plugins/src/main/resources/META-INF/org.apache.grails.buildsrc.grails-code-style/checkstyle/checkstyle.xml Add default Checkstyle config resource
build-logic/plugins/src/main/resources/META-INF/org.apache.grails.buildsrc.grails-code-style/checkstyle/checkstyle-suppressions.xml Add default Checkstyle suppressions resource
build-logic/plugins/src/main/resources/META-INF/org.apache.grails.buildsrc.grails-code-analysis/pmd/pmd.xml Add default PMD ruleset resource
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsViolationAggregationPlugin.groovy Implement root-only aggregation (style + analysis + JaCoCo CSV)
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsCodeStylePlugin.groovy Enhance codestyle plugin (properties, report redirection, codenarcFix)
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsCodeStyleExtension.groovy Update codestyle default directories
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsCodeAnalysisPlugin.groovy Add new PMD/SpotBugs convention plugin
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsCodeAnalysisExtension.groovy Add analysis extension for directories/reports
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GradleUtils.groovy Add booleanProvider helper
build-logic/plugins/build.gradle Add SpotBugs Gradle plugin dependency + test platform config + plugin registrations
.github/workflows/codestyle.yml Switch to aggregated Markdown reports; update artifact publishing
.github/workflows/codeanalysis.yml Add code analysis workflow using aggregation
.agents/skills/violation-fixer/SKILL.md Add contributor documentation for running/fixing violations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .agents/skills/violation-fixer/SKILL.md Outdated
Comment thread .agents/skills/violation-fixer/SKILL.md
Comment thread .github/workflows/codestyle.yml
Comment thread .github/workflows/codeanalysis.yml Outdated
Comment thread .github/workflows/codeanalysis.yml Outdated
- Fix codenarcFix regex bugs (:: method refs, adjacent GStrings, and
  double quotes inside single-quoted strings) and add covering tests
- Wire grails-violation-aggregation into the grails-gradle build so
  aggregateStyleViolations / aggregateAnalysisViolations exist there
- Pass -Pgrails.codeanalysis.enabled.pmd/spotbugs in codeanalysis.yml so
  the workflow actually runs analysis instead of reporting nothing
- Harden the violation-aggregation XmlSlurper against XXE (disallow
  DOCTYPE, disable external general/parameter entities)
- Make createOrLoad idempotent: only rewrite config when missing or the
  on-disk content differs, and log at debug to avoid per-subproject churn
- Remove unused Property import; correct codeanalysis report path in the
  GrailsCodeAnalysisExtension javadoc and the violation-fixer skill doc

Co-authored-by: Walter Duque de Estrada <wbduque@mac.com>
Assisted-by: claude-code:claude-4.7-opus
jamesfredley added a commit that referenced this pull request May 28, 2026
)

Continues shrinking the PR-A review surface by reverting the portions of
this branch split into standalone PRs against 8.0.x, matching the
established B/C/D/E revert pattern. Once these land on 8.0.x and 8.0.x is
merged back into this branch, the reverted changes return through the
merge, so the final state of stage-hibernate7 is unchanged - only the
diff visible on PR-A is reduced.

Reverted content:

  grails-code-analysis convention plugin (PR #15686, PR-G)
    GrailsCodeAnalysisPlugin/Extension, GrailsViolationAggregationPlugin
    (+specs), the codeanalysis workflow, the pmd ruleset resource, the
    codenarcFix improvements and config relocation in GrailsCodeStylePlugin,
    spotbugs/pmd build deps, and the grails-code-analysis apply-line across
    all modules (incl. the h7 clones).

  grails-jacoco convention plugin (PR #15687, PR-H)
    GrailsJacocoPlugin (+spec), the coverage workflow, and the
    grails-jacoco apply-line across all modules (incl. the h7 clones).

NOT reverted (intentionally kept):
  - The codenarc violation fixes (f18465a) - reverting would
    re-introduce style violations; they are plugin-independent.
  - All hibernate7 BOM/clone content (the actual PR-A work).

Verified: ./gradlew help configures the root, grails-gradle, and
grails-forge builds; :build-logic-root:build-logic:test passes.

Assisted-by: claude-code:claude-4.7-opus
Addresses Copilot review feedback: the JaCoCo aggregation previously
hard-coded a filter dropping classes under
org.grails.orm.hibernate.support.hibernate7. This tied a generic
aggregation plugin to a specific package name.

- Introduce grails.jacoco.aggregation.excludedClassPrefixes (comma
  separated), defaulting to the hibernate7 support package. The default
  is required because the Hibernate 5 and Hibernate 7 variants compile
  classes with identical fully-qualified names, and JaCoCo cannot
  aggregate two different classes that share a name.
- Resolve the value as a configuration-cache-safe Provider, register it
  as a task input, and pass it into the parse method.
- Document the property and its rationale on the plugin.
- Add specs covering the default exclusion and the configurable override.

Assisted-by: claude-code:claude-4.7-opus
…lysis-plugin

Resolve conflict in .github/workflows/codestyle.yml: this branch renamed the
forge code-style job to check_forge_projects and switched it to the new
aggregateStyleViolations report flow, so drop the duplicate old-behavior
check_grails_forge_projects job that 8.0.x still appends. Also pin the
check_forge_projects job's actions to the SHA-pinned versions used by the
sibling jobs (checkout v6.0.2, setup-java v5.2.0, setup-gradle v6.1.0 with
cache-provider: basic, upload-artifact v7.0.1) per the ASF action-pinning
policy.

Assisted-by: claude-code:claude-4.8-opus
The codeanalysis.yml workflow used floating action tags (actions/checkout@v6,
actions/setup-java@v4, actions/upload-artifact@v7.0.1) and an outdated
setup-gradle pin, which the ASF org policy rejects, causing the "Code Analysis"
workflow to fail at startup. Pin every action to the same ASF-approved commit
SHAs already used by the other workflows (checkout v6.0.2, setup-java v5.2.0,
setup-gradle v6.1.0 with cache-provider: basic, upload-artifact v7.0.1).

Assisted-by: claude-code:claude-4.8-opus
PMD 6.55.0 cannot parse Java 21+ sources, so pmdMain threw
"PMDException: Error while processing" on every file and never produced
real findings. Bump to PMD 7.25.0, which supports Java 21 through 26
(the Micronaut/Forge modules compile on Java 25).

Run the code-analysis CI jobs with -Pgrails.codeanalysis.ignoreFailures=true
so PMD and SpotBugs complete, the aggregateAnalysisViolations task can run,
and the full violation report is generated and published to the job summary
instead of the build aborting with no report.

This is intentionally non-gating for now: with PMD parsing correctly the
Core build alone reports ~5,016 PMD + 187 SpotBugs violations, so the
ruleset/remediation needs to be scoped before the checks can gate.

Assisted-by: claude-code:claude-opus-4-8
@jamesfredley jamesfredley marked this pull request as draft May 29, 2026 15:15
@jamesfredley
Copy link
Copy Markdown
Contributor Author

Code-analysis status: configuration now surfaces the violations (converted to draft)

I pushed a config change and converted this PR to draft, because once the analysis actually runs it reveals a far larger problem than expected.

Root cause of the previous "red with no report"

pmdVersion was 6.55.0. PMD 6.x cannot parse Java 21+ source, so pmdMain threw PMDException: Error while processing on essentially every .java file and never produced findings. Because the PMD/SpotBugs tasks failed, aggregateAnalysisViolations (which only generates the Markdown report, it does not gate) never ran, so the job was red with No files were found ... build/reports/violations/.

Two changes in the latest commit:

  1. pmdVersion 6.55.0 → 7.25.0 - supports Java 21 through 26 (the Micronaut/Forge modules compile on Java 25).
  2. Code-analysis CI runs report-only (-Pgrails.codeanalysis.ignoreFailures=true) so PMD + SpotBugs complete, the aggregate report is generated, and the full violation list is published to the job summary instead of aborting.

The scope problem

With PMD parsing correctly, the Core build alone reports:

  • 5,016 PMD violations
  • 187 SpotBugs violations

The Forge and Gradle-plugin builds are not measured yet - realistic total is ~8,000-12,000 violations.

Top PMD rules:

Count Rule Nature
1,758 MissingOverride trivial/safe (@Override)
282 LooseCoupling mechanical
264 AvoidCatchingGenericException behavior-risky
247 AvoidLiteralsInIfCondition stylistic
239 AvoidReassigningParameters behavior-risky
203 LiteralsFirstInComparisons mechanical
171 ConstructorCallsOverridableMethod behavior-risky
150 CloseResource behavior-risky
147 AssignmentInOperand mixed
140 UseVarargs mixed

Top SpotBugs bug patterns:

Count Pattern
46 EC_UNRELATED_TYPES_USING_POINTER_EQUALITY
21 DM_DEFAULT_ENCODING
20 RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE
16 MS_SHOULD_BE_FINAL
12 RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT

The ruleset currently enables the entire bestpractices + errorprone + security categories, which is why the count is so high - a large share are pure style preferences, and many of the rest require behavior-changing edits to framework internals.

Recommendation

Fixing 8,000-12,000 locations across the framework in one PR is neither reviewable nor safely mergeable. Suggested path before this can gate:

  1. Curate the PMD ruleset down to a high-signal subset (and review the SpotBugs reportLevel/effort).
  2. Bulk-fix the safe mechanical rules (e.g. the 1,758 MissingOverride).
  3. Baseline or defer the behavior-risky rules to follow-up PRs.

Note: PR #15687 (grails-jacoco) is stacked on top of this branch and inherits these jobs, so it is affected by the same decision.

@jamesfredley jamesfredley force-pushed the feat/grails-code-analysis-plugin branch from be35e3e to 57ae6d6 Compare May 29, 2026 15:40
@jamesfredley
Copy link
Copy Markdown
Contributor Author

Code-style (Checkstyle) status: forge backlog left red on purpose

Separate from the PMD/SpotBugs report above, this stack also turns on Checkstyle gating for the grails-forge modules (the code-style config + grails-forge/gradle/code-style-config.gradle are part of this branch's diff vs 8.0.x). The Core and Gradle-plugin code-style jobs are clean and pass; only Forge Projects fails.

I briefly made the code-style jobs report-only for consistency with the analysis jobs, then reverted it - we are not masking style errors. The Forge Projects code-style job is intentionally left red for now rather than fixed, because it's the same "huge backlog" situation.

The forge Checkstyle backlog: 1,088 errors across 259 files

By module:

Module Files Errors
grails-forge-core 165 731
grails-forge-cli 41 206
grails-forge-api 46 134
grails-forge-analytics-postgres 6 15
grails-forge-web-netty 1 2

By rule:

Count Rule
566 ImportOrder
206 UnusedImports
200 EmptyLineSeparator
37 Indentation
36 SingleSpaceSeparator
25 AvoidStarImport
17 WhitespaceAfter
1 SeparatorWrap

Note

Unlike the PMD/SpotBugs findings, every one of these is a purely mechanical formatting rule - no behavior change. The top three (ImportOrder + UnusedImports + EmptyLineSeparator = ~89%) are exactly what an IDE "Optimize Imports" + reformat, or a Spotless/importOrder pass, would fix automatically. So this backlog is safe to clear in bulk whenever we choose to - it's just out of scope for this draft right now.

PR #15687 is stacked on this branch and inherits the same code-style jobs / red Forge result.

@jamesfredley
Copy link
Copy Markdown
Contributor Author

This will have to wait for Grails 8.1 or 9, there is too much work left.

@borinquenkid
Copy link
Copy Markdown
Member

@jamesfredley For the SpotBugs and PMD, have two skill features one after running report does not break build but updates a predetermined ISSUE with the report. Have another skill called fix SpotBugsOrPmd that picks one and fixes it and creates a PR. If anyone has tokens to spare could create quite a few focused PRS. It would move issues from under the radar to very visible and we can tackle them whenever we have the bandwidth

@jdaugherty
Copy link
Copy Markdown
Contributor

I don't think we can merge hibernate 7 without this change. This is a critical component of that merge. We only have it configured to run in hibernate 7.

private static void doNotApplyStylingToTests(Project project) {
project.tasks.named('checkstyleTest') {
it.enabled = false // Do not check test sources at this time
private static void createOrLoad(Path expectedPath, String defaultResource, Project project) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to this method should be reverted.

@jdaugherty
Copy link
Copy Markdown
Contributor

This PR expanded scope: grails-code-analysis convention plugin (PMD + SpotBugs) applied to 96 subprojects, with GrailsCodeAnalysisExtension. We do not need to do this. The original change was to help implement hibernate 7.

@jamesfredley
Copy link
Copy Markdown
Contributor Author

I now understand that PMD is for 3 subprojects right now and not the entire project.  That changes scope and I'll update and move back from draft.

It appeared there was a large amount of work across the whole codebase and I now see that being possible in our short timeline.

Copy link
Copy Markdown
Member

@borinquenkid borinquenkid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamesfredley @jdaugherty Since this is totally optional in a regular build it should be ok to add. Once we get past 8.0.x I want to add agentic solutions of PMD and Spotbugs one at a time

borinquenkid and others added 2 commits May 30, 2026 15:48
…ted sources

Co-Authored-By: Gemini <noreply@google.com>
- Restrict Spotless to target only production Java files
- Configure Spotless importOrder and removeUnusedImports to match Checkstyle layout conventions
- Remove google-java-format from Spotless configurations to avoid JDK 25 compiler crashes
- Clean up obsolete task references in build configurations
- Fix Java styling and imports across grails-forge production files to satisfy Checkstyle validation

Co-authored-by: Gemini <noreply@google.com>
@borinquenkid
Copy link
Copy Markdown
Member

  1. JDK 25 Compatibility Fix:
    • Removed google-java-format from the grails-forge Spotless configuration, resolving runtime compilation crashes ( NoSuchMethodError ) caused by internal compiler API changes in JDK 25.
  2. Unified Styling & Layout Conventions:
    • Modified Spotless configurations in grails-forge ( gradle/code-style-config.gradle ) to use custom conventions ( importOrder + removeUnusedImports ) to mirror Checkstyle's layout
    requirements.
    • Restricted Spotless scans strictly to production Java source files to prevent conflict with tasks generating resources (e.g. copyGrailsWrapper ).
  3. Enhanced Checkstyle Rules:
    • Configured Checkstyle in the grails-forge project to run layout validation on all production Java source files.
    • Cleared all formatting and import violations across the grails-forge-core , grails-forge-api , and grails-forge-cli modules.
  4. Build Cleanup:
    • Removed obsolete formatting task declarations from the build configurations.

The bespoke Spotless config in grails-forge fought Checkstyle (static
import position and the merged grails import group it cannot express)
and gated checkstyleMain behind spotlessCheck, hiding pre-existing
violations. Remove the Spotless application/config so grails-forge
relies on Checkstyle + CodeNarc via the shared grails-code-style
plugin, matching the root build. Fix the violations this surfaces
(whitespace, import order, annotation-array indentation).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@borinquenkid
Copy link
Copy Markdown
Member

borinquenkid commented May 31, 2026

Update — Spotless removed from grails-forge (supersedes the Spotless changes described above)

While clearing the remaining Checkstyle violations, the importOrder + removeUnusedImports Spotless config turned out to be unable to fully mirror Checkstyle:

  • Spotless's prefix-based importOrder treats grails.* and org.grails.* as separate groups (inserts a blank line between them), while Checkstyle's regex merges them into one group (forbids that blank
    line). No Spotless configuration can express that merged group, so any file importing both (e.g. AddPropertyCommand) was an unsatisfiable conflict — Spotless and Checkstyle could never both pass.
  • Because checkstyleMain depended on spotlessCheck, that conflict (and pre-existing violations elsewhere) were gated/hidden rather than surfaced.

Resolution: removed the bespoke Spotless config from grails-forge entirely. Layout is now owned solely by Checkstyle (verification) + the shared project .idea code-style, which can express the merged
grails group and matches Checkstyle exactly. This puts grails-forge on the same Checkstyle + CodeNarc model as the root build.

  • JDK 25 fix still holds — there's no Java formatter left to hit the NoSuchMethodError.
  • Dropped the checkstyleMain dependsOn spotlessCheck gate; Checkstyle now runs on all production Java unconditionally.
  • Cleared all surfaced violations across grails-forge-core, grails-forge-api, and grails-forge-cli (whitespace, import order, annotation-array indentation).

⚠️ Heads-up: a surprising transitive dependency we hit and deliberately left in place

We could not delete the spotless-plugin-gradle line from grails-forge/buildSrc/build.gradle, even though Spotless is no longer applied. It's load-bearing for dependency resolution, not for formatting:

io.micronaut.build.internal:micronaut-gradle-plugins
└─ com.diffplug.spotless:spotless-plugin-gradle:6.8.0 (old, transitive)
└─ com.diffplug.spotless:spotless-lib-extra:2.27.0
└─ org.codehaus.groovy:groovy-xml:3.0.9 ← Groovy 3 (codehaus)

micronaut-gradle-plugins drags in an old Spotless (6.8.0) that pulls Groovy 3 (org.codehaus.groovy), which collides with the buildSrc's Groovy 4 (org.apache.groovy) and fails resolution. The explicit
spotless-plugin-gradle:6.25.0 we keep forces Spotless up to a Groovy-4-compatible version, overriding the transitive 6.8.0. So the dependency stays purely as a version pin — it is never applied as a
plugin.

Follow-up candidate (out of scope here): replace this implicit pin with an explicit dependency constraint, or exclude the old Spotless from micronaut-gradle-plugins, so the intent is obvious rather than
relying on a leftover implementation line.

Commit: 4149f44

@matrei
Copy link
Copy Markdown
Contributor

matrei commented May 31, 2026

I don't think we can merge hibernate 7 without this change. This is a critical component of that merge. We only have it configured to run in hibernate 7.

@jdaugherty Why is it a critical component of merging an implementation of Hibernate 7?

Also, a general observation, there seems to be a lot of commits with functional and formatting changes combined.
It is helpful if we separate these into different commits.

@jdaugherty
Copy link
Copy Markdown
Contributor

I don't think we can merge hibernate 7 without this change. This is a critical component of that merge. We only have it configured to run in hibernate 7.

@jdaugherty Why is it a critical component of merging an implementation of Hibernate 7?

Also, a general observation, there seems to be a lot of commits with functional and formatting changes combined.

It is helpful if we separate these into different commits.

We used this plugin only in the hibernate 7 dev to ensure security best practices.

Copy link
Copy Markdown
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a first iteration, I did not get through it all.


static Provider<Boolean> booleanProvider(Project project, String name, boolean defaultValue = false) {
project.providers.gradleProperty(name)
.map { Boolean.parseBoolean(it as String) }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use: it.trim().toBoolean()?

final DirectoryProperty pmdDirectory

/**
* Defaults to rootProject.buildDir/reports/codeanalysis.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be project.rootProject, like the one above, or can we remove the project. from both javadoc snippets?

@Inject
GrailsCodeAnalysisExtension(ObjectFactory objects, Project project) {
pmdDirectory = objects.directoryProperty().convention(
project.rootProject.layout.buildDirectory.dir('codeanalysis/pmd')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use directory name code-analysis?

project.rootProject.layout.buildDirectory.dir('codeanalysis/pmd')
)
reportsDirectory = objects.directoryProperty().convention(
project.rootProject.layout.buildDirectory.dir('reports/codeanalysis')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code-analysis?

@CompileStatic
class GrailsCodeAnalysisPlugin implements Plugin<Project> {

static String PMD_DIR_PROPERTY = 'grails.codeanalysis.dir.pmd'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use hyphened code-analysis here and below?

)
}

Provider<Directory> violationsDir = project.layout.buildDirectory.dir('reports/violations')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def here and below?

}

Provider<Directory> violationsDir = project.layout.buildDirectory.dir('reports/violations')
Provider<Directory> styleXmlDir = project.layout.buildDirectory.dir('reports/codestyle')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code-style?


Provider<Directory> violationsDir = project.layout.buildDirectory.dir('reports/violations')
Provider<Directory> styleXmlDir = project.layout.buildDirectory.dir('reports/codestyle')
Provider<Directory> analysisXmlDir = project.layout.buildDirectory.dir('reports/codeanalysis')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code-analysis?

@CompileDynamic
private static void parseStyleViolations(Directory styleXmlDir, Directory violationsDir,
boolean checkStyleTests, boolean codenarcEnabled, boolean checkstyleEnabled) {
def slurper = new XmlSlurper()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract duplicate slurper creation into helper?

!filePath && (className.contains('Spec') || className.contains('Test') || className.contains('Tests'))
}

def writeReport = { String fileName, List violations, String title ->
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is some duplication here?

@testlens-app
Copy link
Copy Markdown

testlens-app Bot commented Jun 1, 2026

✅ All tests passed ✅

🏷️ Commit: 327cdcb
▶️ Tests: 34760 executed
⚪️ Checks: 37/37 completed


Learn more about TestLens at testlens.app.

@matrei
Copy link
Copy Markdown
Contributor

matrei commented Jun 1, 2026

I don't think we can merge hibernate 7 without this change. This is a critical component of that merge. We only have it configured to run in hibernate 7.

@jdaugherty Why is it a critical component of merging an implementation of Hibernate 7?
Also, a general observation, there seems to be a lot of commits with functional and formatting changes combined.
It is helpful if we separate these into different commits.

We used this plugin only in the hibernate 7 dev to ensure security best practices.

That is interesting. I have not used PMD before. Looking at the security rule set, there only seems to be two rules, and none of them seems to apply to Hibernate 7 code. Are there other rules that helps ensure security best practices?

@matrei
Copy link
Copy Markdown
Contributor

matrei commented Jun 1, 2026

Is it a good idea to change Forge styling in this PR?


static Provider<Boolean> booleanProvider(Project project, String name, boolean defaultValue = false) {
project.providers.gradleProperty(name)
.map { String it -> it.trim().toBoolean() }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for explicit it parameter?

.build()

then: "task finished successfully"
result.task(':codenarcFix').outcome == org.gradle.testkit.runner.TaskOutcome.SUCCESS
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary qualified reference? (here and below)

<component name="ProjectCodeStyleConfiguration">
<state>
<option name="USE_PER_PROJECT_SETTINGS" value="true" />
<option name="PREFERRED_PROJECT_CODE_STYLE" value="Default copy" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened here?

</codeStyleSettings>
</code_scheme>
<code_scheme name="Project" version="173">
<option name="AUTODETECT_INDENTS" value="false" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refrain from functional and whitespace changes in the same commit?

@borinquenkid
Copy link
Copy Markdown
Member

Is it a good idea to change Forge styling in this PR?

It was failing CodeStyle!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

6 participants